Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix sql_query_explain.Oracle #1354

Merged
merged 7 commits into from
Dec 21, 2023
Merged

Conversation

thomashulst
Copy link
Contributor

@thomashulst thomashulst commented Aug 11, 2023

Fixes #1353

  • use dbExecute for EXPLAIN PLAN FOR
  • remove extra ) in DBMS_XPLAN.DISPLAY()

@mgirlich
Copy link
Collaborator

sql_query_explain() should not execute a query. I think that sql_query_explain.Oracle() should error with a helpful error instead and you add db_explain.Oracle(). Also please add a method for .

moved query execution from sql_query_explain().Oracle to db_explain.Oracle()
@thomashulst
Copy link
Contributor Author

sql_query_explain() should not execute a query. I think that sql_query_explain.Oracle() should error with a helpful error instead and you add db_explain.Oracle(). Also please add a method for .

Thanks for your pointers, figuring this out as I go. I don't want to impose, but hope to work through this in the hopes of being able to contribute more frequently.

I just submitted something which is hopefully closer to what is intended. Currently db_explain() still falls back to db_explain.DBIConnection(), but I didn't want to touch that before knowing what I submitted is in the right direction.

@hadley
Copy link
Member

hadley commented Nov 2, 2023

It seems like the basic problem is that explaining a query plan in Oracle requires two steps — you first use EXPLAIN PLAN to save the plan to the DBMS_XPLAN.DISPLAY table, and then you need to retrieve the rows from that table. An initial useful contribution would be to document this with links to the Oracle docs, so we don't re-confused when we come back to this code in the future.

You also seem to have split the single SQL statement into two pieces, only one of which is generated by sqll_query_explain, but you don't explain why. If it's because it doesn't work as a single statement, I think it would be slightly clearer for sql_query_explain() to return a character vector, and then you can execute those pieces individually in db_explain().

I think you also need to consider what happens if multiple queries are explained in one session. Won't those plans continue to pile up in the same database? I think it would be useful to consider how you might keep them separate.

@thomashulst
Copy link
Contributor Author

It seems like the basic problem is that explaining a query plan in Oracle requires two steps — you first use EXPLAIN PLAN to save the plan to the DBMS_XPLAN.DISPLAY table, and then you need to retrieve the rows from that table. An initial useful contribution would be to document this with links to the Oracle docs, so we don't re-confused when we come back to this code in the future.

I'll link to the Oracle documentation of the current long term release (19c). The gist of explaining and displaying execution plans in Oracle can be found here.

Basically, EXPLAIN PLAN inserts a row with the execution plan for a particular statement into the PLAN_TABLE. PLAN_TABLE is a global temporary table automatically set up for each user and the default output table of EXPLAIN PLAN. Then, DBMS_XPLAN.DISPLAY is used to display the execution plan. It's a function that takes a few arguments, but by default it selects the most recent execution plan from PLAN_TABLE and returns it nicely formatted.

You also seem to have split the single SQL statement into two pieces, only one of which is generated by sqll_query_explain, but you don't explain why. If it's because it doesn't work as a single statement, I think it would be slightly clearer for sql_query_explain() to return a character vector, and then you can execute those pieces individually in db_explain().

Exactly, it doesn't work as a single statement. I see why returning a character vector might be nicer and will submit that shortly. Again, db_explain still falls back to db_explain.DBIConnection(), but the commit will hopefully be closer to what is expected.

I think you also need to consider what happens if multiple queries are explained in one session. Won't those plans continue to pile up in the same database? I think it would be useful to consider how you might keep them separate.

In theory you could specify a unique value for the STATEMENT_ID when using EXPLAIN PLAN and then retrieving it with DBMS_XPLAN.DISPLAY(statement_id => unique_id). However, the DBMS_XPLAN.DISPLAY function selects the most recently generated execution plan by default, so in my mind it takes care of this problem. Furthermore, PLAN_TABLE is dropped at the end of each session, so no need to worry about plans piling up.

Thanks again for your patience and all helpful pointers.

…d DBMS_XPLAN.DISPLAY and executes those individually in db_explain
@hadley
Copy link
Member

hadley commented Nov 10, 2023

Thanks for the explanations!

@hadley hadley merged commit 267a12e into tidyverse:main Dec 21, 2023
13 checks passed
@hadley
Copy link
Member

hadley commented Dec 21, 2023

@thomashulst thanks so much for this PR!

@thomashulst
Copy link
Contributor Author

Happy to contribute, I learned a lot from it 😊

@thomashulst thomashulst deleted the fix-oracle-explain branch December 21, 2023 15:50
@hadley
Copy link
Member

hadley commented Dec 21, 2023

If you're interested in tackling another Oracle-dbplyr issue, I think #750 looks fairly straightforward.

@thomashulst thomashulst restored the fix-oracle-explain branch December 22, 2023 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Oracle explain() SQL command not properly ended
3 participants